-
Notifications
You must be signed in to change notification settings - Fork 5.5k
[Component] InMobile - Send SMS messages #18760
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
WalkthroughAdds an InMobile "Send SMS Messages" action, safe JSON/array parsing utilities, and refactors the InMobile app to centralize HTTP requests (including a sendSms method). Also bumps package version and adds an @pipedream/platform dependency. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant A as Action: send-sms-messages
participant UTL as Utils
participant APP as InMobile App
participant API as InMobile REST API
U->>A: invoke action with `app` + `messages`
A->>UTL: parseArray(messages)
UTL-->>A: parsed messages[]
A->>APP: sendSms({ messages: parsed messages })
APP->>APP: getAuth() / getUrl("/sms/outgoing")
APP->>API: POST /sms/outgoing (auth + payload)
API-->>APP: response
APP-->>A: response
A-->>U: returns response and summary ("N messages sent")
note right of APP: makeRequest/post centralize HTTP calls
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Nitpick comments (1)
components/inmobile/common/utils.mjs (1)
58-60
: Flatten nested arrays and decrement depth in parseArrayEnsure consistent depth handling and normalize nested arrays (common when strings contain JSON arrays).
Apply this diff:
- if (Array.isArray(input)) { - return input.map((item) => parseArray(item, maxDepth)); - } + if (Array.isArray(input)) { + return input.flatMap((item) => { + const v = parseArray(item, maxDepth - 1); + return Array.isArray(v) ? v : [v]; + }); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
components/inmobile/actions/send-sms-messages/send-sms-messages.mjs
(1 hunks)components/inmobile/common/utils.mjs
(1 hunks)components/inmobile/inmobile.app.mjs
(1 hunks)components/inmobile/package.json
(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Lint Code Base
- GitHub Check: Publish TypeScript components
- GitHub Check: Verify TypeScript components
- GitHub Check: pnpm publish
🔇 Additional comments (1)
components/inmobile/package.json (1)
3-3
: Version bump and platform dependency look goodNo issues spotted. Please confirm the runtime uses @pipedream/platform >= 3.1.x to avoid runtime mismatches.
Also applies to: 15-17
2e5902a
to
b512325
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
♻️ Duplicate comments (2)
components/inmobile/actions/send-sms-messages/send-sms-messages.mjs (2)
18-55
: Critical: Prop type contradicts documentation and example.The
messages
prop is declared asstring[]
(line 18), but the entire description (lines 20-54) documents message objects with fields liketo
,text
,from
, etc., and the example (lines 39-53) shows an array of objects—not strings.This type mismatch will confuse users and break at runtime when the action attempts to use string elements as objects.
Resolution options:
- Preferred: Change the prop type to accept objects directly so users can pass
[{ to: "...", text: "...", from: "..." }]
without manual JSON stringification.- Alternative: Keep
string[]
but clarify that each string must be a valid JSON object (not a top-level JSON array), and update the example to show individual stringified objects.Would you like me to generate the updated prop definition for option 1?
63-71
: Critical: Missing validation, incorrect count, and nested array risk.The current implementation has multiple critical issues:
No flattening (line 66):
utils.parseArray(messages)
can return nested arrays if any element is a JSON array string (e.g.,["[{...}, {...}]"]
becomes[[{...}, {...}]]
), breaking the API call.No message count validation: The InMobile API requires 1-250 messages. This is not enforced.
No required field validation: The code doesn't check that each message has
to
,text
, andfrom
fields, leading to cryptic API errors.Incorrect summary (line 70): Uses original
messages.length
instead of the parsed/flattened count, so the summary may report the wrong number.No idempotency safeguard: Without enforcing or auto-assigning
messageId
, retries will send duplicate messages.Apply this diff to address all issues:
+ // Normalize inputs: accept string[], object[], or JSON array strings within the array + const normalizedMessages = (Array.isArray(messages) ? messages : [messages]) + .flatMap((m) => { + const v = utils.parseArray(m); + return Array.isArray(v) ? v : [v]; + }); + + if (normalizedMessages.length < 1 || normalizedMessages.length > 250) { + throw new Error("Messages must contain between 1 and 250 items."); + } + normalizedMessages.forEach((msg, i) => { + if (!msg || typeof msg !== "object") { + throw new Error(`Message at index ${i} is not an object.`); + } + const { to, text, from } = msg; + if (!to || !text || !from) { + throw new Error(`Message at index ${i} is missing required field(s): to, text, from.`); + } + }); + const response = await app.sendSms({ $, data: { - messages: utils.parseArray(messages), + messages: normalizedMessages, }, }); - $.export("$summary", `Successfully sent ${messages.length} SMS message(s)`); + $.export("$summary", `Successfully sent ${normalizedMessages.length} SMS message(s)`); return response;Note on idempotency: Consider requiring or auto-assigning
messageId
for each message to prevent duplicate sends on retries.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
components/inmobile/actions/send-sms-messages/send-sms-messages.mjs
(1 hunks)components/inmobile/common/utils.mjs
(1 hunks)components/inmobile/inmobile.app.mjs
(1 hunks)components/inmobile/package.json
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/inmobile/package.json
- components/inmobile/inmobile.app.mjs
🧰 Additional context used
🧬 Code graph analysis (1)
components/inmobile/common/utils.mjs (1)
components/google_cloud/sources/bigquery-query-results/bigquery-query-results.mjs (1)
key
(48-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: pnpm publish
- GitHub Check: Lint Code Base
- GitHub Check: Publish TypeScript components
- GitHub Check: Verify TypeScript components
🔇 Additional comments (1)
components/inmobile/common/utils.mjs (1)
65-68
: LGTM!The default export correctly exposes both utility functions.
WHY
Resolves #9188
Summary by CodeRabbit
New Features
Refactor
Chores